refactor: decouple executor from storage layer by retran · Pull Request #238 · mendixlabs/mxcli

retran · 2026-04-19T17:37:19Z

Why

After PRs #235–#237, the executor still holds direct references to *mpr.Reader and *mpr.Writer in its struct, and several command files still construct BSON for datagrids, filters, and widget cloning. This PR completes the decoupling: the executor works primarily through the backend.FullBackend abstraction.

This is the architectural pivot — dependency inversion via a BackendFactory pattern replaces the direct MPR reader/writer construction.

What changed (incremental from PR #237)

32 files changed, +1,523/-3,121 (net -1,598 lines)

Executor struct decoupled:

Removed writer and reader fields from the Executor struct
Added BackendFactory injection — executor_connect.go rewritten to create backends through the factory instead of constructing mpr.Reader/mpr.Writer directly
cmd/mxcli/main.go updated to inject the factory

Remaining BSON extracted:

mdl/backend/mpr/datagrid_builder.go (1,260 lines) — DataGrid2 construction, filter widgets, column building moved from executor to backend
mdl/backend/mpr/datagrid_builder_test.go — test coverage for datagrid building

Deleted from executor (5 files, ~1,800 lines):

bson_helpers.go — BSON utility functions
cmd_pages_builder_input_cloning.go — widget cloning logic
cmd_pages_builder_input_datagrid.go — datagrid construction
cmd_pages_builder_input_filters.go — filter widget construction
cmd_pages_builder_v3_pluggable.go — pluggable widget BSON

Result: The only BSON remaining in the executor is in 3 read-only files (describe, diff) that deserialize for display — no write-path BSON. Reader() remains as an escape hatch for these read-only paths.

Review-driven improvements

Mock defaults: OpenPageForMutation, OpenWorkflowForMutation, BuildDataGrid2Widget, BuildFilterWidget now return descriptive errors instead of (nil, nil)
Nil guards: updateWidgetsInContainer checks for nil mutator; ParseMicroflowFromRaw returns stub MDL on nil
Spec type aliasing: NavigationProfileSpec, NavHomePageSpec, NavMenuItemSpec in sdk/mpr are now type aliases of mdl/types — eliminates silent divergence risk
Field-count drift assertions: TestFieldCountDrift with 30 assertions (15 struct pairs) catches field additions/removals in manually-copied types
Disconnect error propagation: execConnect, reconnect, execDisconnect log disconnect errors; Executor.Close() returns disconnect error
Nil backendFactory guard: reconnect checks for nil factory before attempting reconnection
Stale comments: "underlying reader" → "backend" in cmd_pages_builder.go
Unused param: buildDataGrid2Property receiver renamed to _

Stack

PR 4/5 in the shared-types refactoring stack.
Merge order: #232 → #235 → #236 → #237 → this → #239

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Refactors the MDL executor to stop manipulating sdk/mpr/BSON directly and instead operate through backend abstractions (including new mutation interfaces), while migrating various shared value types into mdl/types.

Changes:

Replaced many sdk/mpr dependencies with mdl/types and backend interface calls across executor and catalog.
Introduced backend mutation interfaces (PageMutator, WorkflowMutator, widget builder/serialization backends) and updated widget update flow to use them.
Added extensive MockBackend-based executor tests and adjusted CLI/example wiring to set a backend factory.

Reviewed changes

Copilot reviewed 109 out of 148 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
mdl/executor/cmd_workflows_write.go	Switch workflow UUID/ID generation to `mdl/types`.
mdl/executor/cmd_workflows_mock_test.go	Add mock-backend tests for workflow show/describe.
mdl/executor/cmd_widgets.go	Replace raw BSON widget updates with `PageMutator`.
mdl/executor/cmd_settings_mock_test.go	Add mock-backend tests for settings show/describe.
mdl/executor/cmd_security_write.go	Migrate security write types from `mpr` to `types`.
mdl/executor/cmd_security_mock_test.go	Add mock-backend tests for security show/describe paths.
mdl/executor/cmd_rest_clients_mock_test.go	Add mock-backend tests for REST clients show/describe.
mdl/executor/cmd_rename.go	Convert rename hit types from `mpr` to `types`.
mdl/executor/cmd_published_rest_mock_test.go	Add mock-backend tests for published REST services.
mdl/executor/cmd_pages_mock_test.go	Add mock-backend tests for pages/snippets/layouts list.
mdl/executor/cmd_pages_create_v3.go	Build pages/snippets via backend-backed `pageBuilder`.
mdl/executor/cmd_pages_builder_v3_pluggable.go	Remove BSON-based pluggable widget builder helpers.
mdl/executor/cmd_pages_builder_v3_layout.go	Swap `mpr.GenerateID` usages to `types.GenerateID`.
mdl/executor/cmd_pages_builder_input_filters.go	Remove BSON filter widget construction helpers.
mdl/executor/cmd_pages_builder_input_cloning.go	Remove BSON deep-clone helpers from executor.
mdl/executor/cmd_pages_builder_input.go	Move snippet resolution to backend list APIs.
mdl/executor/cmd_pages_builder.go	Refactor `pageBuilder` to use `backend.FullBackend` + `mdl/types`.
mdl/executor/cmd_odata_mock_test.go	Add mock-backend tests for OData show/describe.
mdl/executor/cmd_odata.go	Replace `mpr` helpers with `types` (IDs, EDMX parsing).
mdl/executor/cmd_notconnected_mock_test.go	Add tests for “not connected” behavior across commands.
mdl/executor/cmd_navigation_mock_test.go	Add mock-backend tests for navigation show/describe.
mdl/executor/cmd_navigation.go	Switch navigation document/spec types to `mdl/types`.
mdl/executor/cmd_modules_mock_test.go	Add mock-backend tests for module listing aggregation.
mdl/executor/cmd_misc_mock_test.go	Add mock-backend test for version output.
mdl/executor/cmd_microflows_mock_test.go	Add mock-backend tests for microflow show/describe.
mdl/executor/cmd_microflows_create.go	Switch IDs to `types.GenerateID`; use backend for lookups.
mdl/executor/cmd_microflows_builder_workflow.go	Switch microflow IDs to `types.GenerateID`.
mdl/executor/cmd_microflows_builder_graph.go	Switch graph element IDs to `types.GenerateID`.
mdl/executor/cmd_microflows_builder_flows.go	Switch sequence flow IDs to `types.GenerateID`.
mdl/executor/cmd_microflows_builder_control.go	Switch control structure IDs to `types.GenerateID`.
mdl/executor/cmd_microflows_builder_annotations.go	Switch annotation IDs to `types.GenerateID`.
mdl/executor/cmd_microflows_builder.go	Generalize microflow builder “reader” to backend interface.
mdl/executor/cmd_mermaid_mock_test.go	Add mock-backend test for mermaid domain model describe.
mdl/executor/cmd_lint.go	Route lint reader via executor accessor.
mdl/executor/cmd_jsonstructures_mock_test.go	Add mock-backend tests for JSON structures show.
mdl/executor/cmd_jsonstructures.go	Migrate JSON structure helpers/types to `mdl/types`.
mdl/executor/cmd_javascript_actions_mock_test.go	Add mock-backend tests for JavaScript actions.
mdl/executor/cmd_javaactions_mock_test.go	Add mock-backend tests for Java actions.
mdl/executor/cmd_javaactions.go	Migrate Java action creation IDs to `types.GenerateID`.
mdl/executor/cmd_import_mappings_mock_test.go	Add mock-backend tests for import mappings show.
mdl/executor/cmd_import_mappings.go	Switch JSON element typing to `types` and use backend for DM lookup.
mdl/executor/cmd_imagecollections_mock_test.go	Add mock-backend tests for image collections.
mdl/executor/cmd_imagecollections.go	Switch image collection typings to `mdl/types`.
mdl/executor/cmd_fragments_mock_test.go	Add tests for fragment listing behavior.
mdl/executor/cmd_folders.go	Switch folder info typing to `mdl/types`.
mdl/executor/cmd_features.go	Update comment to reflect “not connected” behavior.
mdl/executor/cmd_export_mappings_mock_test.go	Add mock-backend tests for export mappings show.
mdl/executor/cmd_export_mappings.go	Switch JSON element typing to `types` and use backend for DM lookup.
mdl/executor/cmd_enumerations_mock_test.go	Refactor + expand mock tests for enumerations show/describe.
mdl/executor/cmd_entities_mock_test.go	Add mock-backend tests for entity listing.
mdl/executor/cmd_entities.go	Switch entity creation IDs to `types.GenerateID`.
mdl/executor/cmd_diff_local.go	Delegate microflow parsing from raw BSON map to backend.
mdl/executor/cmd_dbconnection_mock_test.go	Add mock-backend tests for database connections show/describe.
mdl/executor/cmd_datatransformer_mock_test.go	Add mock-backend tests for data transformers show/describe.
mdl/executor/cmd_contract.go	Switch EDMX/AsyncAPI parsing and types to `mdl/types`.
mdl/executor/cmd_constants_mock_test.go	Add mock-backend tests for constants show/describe.
mdl/executor/cmd_catalog.go	Update local executor creation to use `backend` field.
mdl/executor/cmd_businessevents_mock_test.go	Add mock-backend tests for business event services.
mdl/executor/cmd_businessevents.go	Switch channel ID generation to `types.GenerateID`.
mdl/executor/cmd_associations_mock_test.go	Add mock-backend tests for associations show.
mdl/executor/cmd_agenteditor_mock_test.go	Add mock-backend tests for agent editor documents.
mdl/catalog/builder_references.go	Use `types.NavMenuItem` in reference extraction.
mdl/catalog/builder_navigation.go	Use `types.NavMenuItem` in navigation indexing.
mdl/catalog/builder_contract.go	Switch contract parsing to `types.ParseEdmx` / `types.ParseAsyncAPI`.
mdl/catalog/builder.go	Update CatalogReader interface to use `mdl/types` shared structs.
mdl/bsonutil/bsonutil.go	Add BSON ID conversion utilities without `sdk/mpr` dependency.
mdl/backend/workflow.go	Replace image backend signatures to use `mdl/types`.
mdl/backend/security.go	Replace security member access/revocation types to `mdl/types`.
mdl/backend/navigation.go	Replace navigation backend types/specs to `mdl/types`.
mdl/backend/mutation.go	Introduce mutation + widget builder/serialization backend interfaces.
mdl/backend/mpr/datagrid_builder_test.go	Move deep-clone BSON tests into mpr backend package, using `bsonutil`.
mdl/backend/mock/mock_workflow.go	Update mock image backend signatures to `mdl/types`.
mdl/backend/mock/mock_security.go	Update mock signatures to use `mdl/types`.
mdl/backend/mock/mock_navigation.go	Update mock signatures to use `mdl/types`.
mdl/backend/mock/mock_mutation.go	Add mock implementations for new mutation/builder interfaces.
mdl/backend/mock/mock_module.go	Update mock folder list signatures to `mdl/types`.
mdl/backend/mock/mock_microflow.go	Add mock hook for `ParseMicroflowFromRaw`.
mdl/backend/mock/mock_mapping.go	Update JSON structure signatures to `mdl/types`.
mdl/backend/mock/mock_java.go	Update Java/JS action signatures to `mdl/types`.
mdl/backend/mock/mock_infrastructure.go	Port raw unit/rename/widget type signatures to `mdl/types`; expand agent editor mocks.
mdl/backend/mock/mock_connection.go	Switch version/project version types to `mdl/types`.
mdl/backend/mock/backend.go	Extend MockBackend struct for new types + mutation/builder hooks.
mdl/backend/microflow.go	Add backend-level `ParseMicroflowFromRaw` API.
mdl/backend/mapping.go	Port mapping backend JSON structures API to `mdl/types`.
mdl/backend/java.go	Port Java backend typed list/read APIs to `mdl/types`.
mdl/backend/infrastructure.go	Port rename/raw unit/widget type APIs to `mdl/types`.
mdl/backend/doc.go	Update package docs to reflect `mdl/types` shared types.
mdl/backend/connection.go	Port connection + folder backend types to `mdl/types`.
mdl/backend/backend.go	Extend FullBackend with mutation/builder/serialization interfaces.
examples/create_datagrid2_page/main.go	Wire executor to mpr backend via backend factory.
cmd/mxcli/project_tree.go	Switch menu tree node input type to `types.NavMenuItem`.
cmd/mxcli/main.go	Wire CLI executor to mpr backend via backend factory.

Comments suppressed due to low confidence (1)

mdl/executor/cmd_diff_local.go:503

microflowBsonToMDL claims it “falls back to a header-only stub if parsing fails”, but it does not handle ParseMicroflowFromRaw returning nil. If the backend cannot parse the raw map (or a mock returns nil), renderMicroflowMDL is likely to panic. Add an explicit nil check and return the intended stub output (or an empty/diagnostic MDL representation) when parsing returns nil.

func microflowBsonToMDL(ctx *ExecContext, raw map[string]any, qualifiedName string) string {
	qn := splitQualifiedName(qualifiedName)
	mf := ctx.Backend.ParseMicroflowFromRaw(raw, model.ID(qn.Name), "")

	entityNames, microflowNames := buildNameLookups(ctx)
	return renderMicroflowMDL(ctx, mf, qn, entityNames, microflowNames, nil)
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

github-actions · 2026-04-20T14:01:06Z

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

In mdl/executor/cmd_diff_local.go, the null check for parsed microflow returns a hardcoded string format. While this improves error handling, it might be better to propagate the error up the call stack for consistent error handling. However, given this is in a describe/diff context where output formatting is expected, this approach is acceptable.
The MprReader() method in mdl/backend/mpr/backend.go provides direct SDK access as an escape hatch. While the comment advises preferring Backend methods, this could potentially be misused. However, this is a reasonable trade-off for gradual migration and certain edge cases (like linter rules mentioned in the comment).

What Looks Good

Clean dependency inversion: Executor now depends on backend.FullBackend abstraction via BackendFactory instead of direct sdk/mpr imports
Successful relocation of BSON construction logic: Datagrid2, filter widget, and cloning logic moved from executor to backend (mdl/backend/mpr/datagrid_builder.go)
Test coverage maintained: Datagrid builder tests moved and renamed appropriately
Consistent application: Changes applied uniformly across executor command files, examples, and main CLI
Improved error handling: setWidgetCaptionMut now returns proper error instead of silent failure
Cleanup: Removed unused BSON helper files (bson_helpers.go, widget cloning/files)
Proper interface implementation: Mock backend updated with new widget builder methods
Architectural alignment: Fully implements the dependency inversion pattern described in CLAUDE.md

Recommendation

Approve the PR. This refactor successfully completes the executor decoupling from the storage layer as intended. All changes are focused, consistent with the architectural direction, maintain test coverage, and improve the codebase without introducing functional changes. The PR addresses technical debt while preserving existing functionality through proper abstraction boundaries.

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

Copilot

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

retran · 2026-04-20T14:30:35Z

Thanks for the review.

Microflow null check — agreed, the hardcoded string format is intentional here. This is a describe/diff context where we render a stub MDL representation when parsing fails. Propagating an error would suppress the rest of the diff output.
MprReader() escape hatch — correct, this is a deliberate transitional API. The comment on the method documents the intent and advises callers to prefer Backend methods. It will be removed once linter rules and remaining edge cases are migrated.

github-actions · 2026-04-20T14:30:41Z

AI Code Review

What Looks Good

This PR successfully accomplishes its goal of decoupling the executor from the storage layer through dependency inversion. Key strengths:

Clean architectural separation: The Executor no longer holds direct references to *mpr.Reader and *mpr.Writer, instead working through the backend.FullBackend abstraction via a BackendFactory pattern.
Complete BSON extraction: All write-path BSON construction (DataGrid2, filters, widget cloning) has been moved from the executor to the backend (mdl/backend/mpr/datagrid_builder.go), leaving only read-only BSON in the executor for describe/diff operations.
Consistent implementation: Changes are applied uniformly across:
- Executor struct (executor.go)
- Connection setup (executor_connect.go)
- Main CLI entrypoint (main.go)
- Examples (create_datagrid2_page/main.go)
- Command files (updated to use ctx.Backend instead of direct reader/writer access)
- Mock backend (updated with new widget building methods)
Test coverage maintained: The datagrid builder test was properly relocated and adapted from the executor test file.
Minimal disruption: The refactor focuses purely on dependency inversion without changing behavior - the executor still functions correctly through the backend abstraction.

The PR represents a significant improvement in separation of concerns, making the executor storage-agnostic and easier to test/maintain. All changes support the single stated goal of architectural decoupling.

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

github-actions · 2026-04-20T14:35:09Z

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

In mdl/executor/cmd_diff_local.go, the added nil check for microflow parsing is reasonable but could be improved with more context about why parsing might fail. However, this is a minor defensive addition that doesn't block approval.
The mock backend now returns errors for unconfigured methods instead of nil, which is good for testability but may require updates in test files that use the mock backend (not visible in the diff).

What Looks Good

Successfully decouples executor from storage layer by removing writer and reader fields from Executor struct
Properly implements BackendFactory pattern for dependency injection
Moves complex BSON construction (DataGrid2, filters, widget cloning) from executor to backend where it belongs
Maintains test coverage by moving/renaming test files appropriately
Preserves error handling and adds appropriate error messages in mock implementations
Updates all call sites consistently (executor, examples, mutation.go, etc.)
Aligns with architectural goal of dependency inversion via backend abstraction
Part of a planned refactor stack (PR 4/5 in shared-types refactoring)
The only BSON remaining in executor is in read-only files (describe, diff) for display purposes
No MDL syntax changes were introduced, so full-stack consistency checklist for MDL features doesn't apply

Recommendation

Approve this PR. The refactor successfully accomplishes its goal of decoupling the executor from the storage layer through the BackendFactory pattern, moves BSON construction responsibilities to the appropriate backend layer, maintains test coverage, and follows the project's architectural direction. The changes are focused, consistent, and improve the codebase's separation of concerns.

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

Copilot

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

mdl/executor/hierarchy.go:68

newContainerHierarchyImpl ignores errors from ListUnits and ListFolders. If either call fails (e.g., transient backend error), the hierarchy cache is silently incomplete, which can lead to incorrect module/folder resolution later. Consider returning the error (or at least logging) so callers can fail fast instead of building a partial hierarchy.
mdl/executor/executor.go:334
Executor.Close() always returns nil even if backend.Disconnect() fails. Since Disconnect() can return an error, propagate it (or wrap it) so callers can detect close/flush failures.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

github-actions · 2026-04-20T14:38:23Z

AI Code Review

Critical Issues
None found.

Moderate Issues
None found.

Minor Issues
None found.

What Looks Good

Successfully decouples executor from storage layer via BackendFactory pattern
Removes ~1,800 lines of BSON logic from executor files
Moves datagrid/filter/widget cloning logic to appropriate backend location
Maintains test coverage (datagrid_builder_test.go moved with 91% similarity)
Clean dependency injection: executor works through backend.FullBackend abstraction
Preserves error handling patterns using mdlerrors
Clear architectural pivot as described in PR: "dependency inversion via a BackendFactory pattern replaces the direct MPR reader/writer construction"
Consistent application across 32 files with net line reduction (-1,598)
No MDL syntax changes (appropriate for this refactor-focused PR)
Follows single responsibility principle - each change supports the decoupling goal

Recommendation
Approve - this PR successfully completes the architectural refactor to decouple the executor from direct MPR reader/writer dependencies through proper backend abstraction and dependency injection. The changes are focused, well-tested, and improve separation of concerns without introducing functionality gaps.

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

github-actions · 2026-04-20T14:42:56Z

AI Code Review

What Looks Good

This PR successfully executes the architectural pivot to decouple the executor from the storage layer via dependency inversion. The changes are consistent, focused, and maintain backward compatibility where needed:

Clean dependency injection: The BackendFactory pattern is properly implemented in main.go and examples, replacing direct mpr.Reader/mpr.Writer construction with abstraction-backed backend creation.
Complete BSON migration: All datagrid, filter, and widget cloning BSON logic has been moved from executor to backend (datagrid_builder.go), leaving only read-only BSON in executor for describe/diff operations.
Thoughtful escape hatch: The MprReader() method on MprBackend provides a controlled escape for legacy code (like linters) while guiding new code toward the FullBackend interface.
Consistent interface updates: All call sites were updated to use backend.FullBackend instead of direct SDK types (e.g., in cmd_export_mappings.go, cmd_alter_page.go).
Improved error handling: Mock backend now returns descriptive errors instead of nil for unimplemented methods, increasing robustness.
Test coverage maintained: Test files were appropriately moved/renamed (e.g., cmd_pages_builder_input_cloning_test.go → datagrid_builder_test.go).
Field safety: Added field-count drift tests to prevent silent API mismatches in conversion code.
Gradual migration path: The refactor preserves existing functionality while enabling future storage backends through the BackendFactory.

Recommendation

Approve. This PR is a well-executed, focused refactor that achieves its stated goal of decoupling the executor from the storage layer. The changes are consistent with the project's architectural direction, maintain test coverage, improve error handling, and provide a clean migration path. No critical issues were identified in the reviewed diff.

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

Copilot

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

github-actions · 2026-04-20T14:47:39Z

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

In mdl/executor/cmd_diff_local.go, the error handling for failed microflow parsing returns a formatted string but doesn't use the standard error types from mdlerrors. This isn't critical but could be improved for consistency.
In mdl/backend/mpr/backend.go, the New() function comment says "Call Connect(path) to open a project" but there's no Connect method shown in the diff - this may be elsewhere in the file or the comment could be slightly misleading.

What Looks Good

Excellent execution of the architectural refactor: completely removed direct *mpr.Reader and *mpr.Writer references from the Executor struct
Properly implemented BackendFactory pattern for dependency injection
Successfully migrated all BSON construction logic (DataGrid2, filters, widget cloning) from executor to backend layer
Maintained test coverage by moving test files along with implementation
Updated all callers (main.go, examples) to use the new factory pattern
Preserved error handling and functionality throughout the refactor
The change enables better separation of concerns and testability
Only read-only BSON remains in executor (for describe/diff) as intended
Clean removal of ~1,800 lines of BSON construction code from executor files
Consistent application of the pattern across all affected files

Recommendation

Approve. This PR successfully completes the executor decoupling refactor as described in the architectural plan. It moves storage-layer-specific BSON construction to the appropriate backend layer while maintaining all functionality through the backend.FullBackend abstraction. The changes are focused, well-tested, and follow the established patterns from previous PRs in the stack. Minor issues noted do not block approval.

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

Copilot

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Replace *mpr.Reader/*mpr.Writer with backend.FullBackend throughout executor. Inject BackendFactory to remove mprbackend import from executor_connect.go. Move all remaining write-path BSON construction (DataGrid2, filters, cloning, widget property updates) behind backend interface. - Remove writer/reader fields from Executor struct - Add BackendFactory injection pattern for connect/disconnect - Create mdl/backend/mpr/datagrid_builder.go (1260 lines) - Add BuildDataGrid2Widget, BuildFilterWidget to WidgetBuilderBackend - Delete bson_helpers.go, cmd_pages_builder_input_cloning.go, cmd_pages_builder_input_datagrid.go, cmd_pages_builder_v3_pluggable.go - Remaining BSON: 3 read-only files (describe, diff) — WASM-safe

github-actions · 2026-04-20T14:58:38Z

AI Code Review

What Looks Good

This PR successfully accomplishes its goal of decoupling the executor from the storage layer through dependency inversion. Key strengths:

Clean architectural separation: The executor now works exclusively through the backend.FullBackend abstraction, with direct MPR reader/writer references removed
Proper dependency injection: BackendFactory pattern implemented correctly in main.go and examples
Complete BSON migration: All datagrid, filter, and widget cloning BSON construction moved to mdl/backend/mpr/datagrid_builder.go (1,260 lines) with corresponding test file
Thoughtful escape hatch: MprReader() method added for legacy cases while encouraging backend abstraction use
Interface evolution: WidgetBuilderBackend properly extended with new methods (BuildDataGrid2Widget, BuildFilterWidget) and supporting structs
Consistent refactor: All call sites updated (alter_page, export_mappings, etc.) to use backend abstraction
Test coverage maintained: Datagrid builder test file relocated and preserved
Focused scope: Purely an architectural refactor with no mixed concerns
Error handling preserved: Proper use of mdlerrors throughout moved code

Recommendation

Approve - This PR is a well-executed architectural refactor that successfully decouples the executor from storage layer dependencies. It follows the dependency inversion principle, maintains test coverage, updates all consumers consistently, and prepares the codebase for future shared-types work as described in the PR stack. No blocking issues identified.

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

Copilot AI review requested due to automatic review settings April 19, 2026 17:37

This was referenced Apr 19, 2026

refactor: code quality — deterministic output, doc comments, naming #239

Open

refactor: define mutation backend interfaces #236

Merged

Copilot AI reviewed Apr 19, 2026

View reviewed changes

Comment thread mdl/executor/cmd_widgets.go

Comment thread mdl/backend/mock/mock_mutation.go

Comment thread mdl/bsonutil/bsonutil.go

Comment thread mdl/executor/cmd_pages_builder.go Outdated

This was referenced Apr 19, 2026

refactor: extract shared types and utility functions to mdl/types #235

Merged

refactor: implement mutation backends and migrate handlers #237

Merged

retran marked this pull request as draft April 19, 2026 18:09

retran force-pushed the feature/decouple-executor branch 2 times, most recently from 07c9ad9 to e275c0f Compare April 20, 2026 08:01

github-actions bot mentioned this pull request Apr 20, 2026

Daily Digest: 2026-04-20 #240

Closed

retran force-pushed the feature/decouple-executor branch from e275c0f to 66f637a Compare April 20, 2026 14:00

retran requested a review from Copilot April 20, 2026 14:02

Copilot started reviewing on behalf of retran April 20, 2026 14:03 View session

retran force-pushed the feature/decouple-executor branch from 66f637a to 62069b1 Compare April 20, 2026 14:07

Copilot AI reviewed Apr 20, 2026

View reviewed changes

Comment thread mdl/backend/mpr/datagrid_builder.go Outdated

Comment thread mdl/backend/mock/mock_mutation.go Outdated

Comment thread mdl/executor/executor_connect.go

Comment thread mdl/executor/executor.go

retran requested a review from Copilot April 20, 2026 14:30

Copilot started reviewing on behalf of retran April 20, 2026 14:31 View session

retran force-pushed the feature/decouple-executor branch from 3d79a42 to bc18c2b Compare April 20, 2026 14:33

Copilot AI reviewed Apr 20, 2026

View reviewed changes

Comment thread mdl/executor/executor_connect.go

Comment thread mdl/executor/executor_connect.go

Comment thread mdl/executor/executor_connect.go Outdated

retran requested a review from Copilot April 20, 2026 14:38

Copilot started reviewing on behalf of retran April 20, 2026 14:38 View session

retran force-pushed the feature/decouple-executor branch from de25c84 to f0cf570 Compare April 20, 2026 14:41

Copilot AI reviewed Apr 20, 2026

View reviewed changes

Comment thread mdl/executor/executor.go

retran requested a review from Copilot April 20, 2026 14:46

Copilot started reviewing on behalf of retran April 20, 2026 14:47 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

retran marked this pull request as ready for review April 20, 2026 14:54

retran force-pushed the feature/decouple-executor branch from 4d2582a to 18ff068 Compare April 20, 2026 14:57

Conversation

retran commented Apr 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changed (incremental from PR #237)

Review-driven improvements

Stack

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 20, 2026

AI Code Review

Critical Issues

Moderate Issues

Minor Issues

What Looks Good

Recommendation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

retran commented Apr 20, 2026

Uh oh!

github-actions bot commented Apr 20, 2026

AI Code Review

What Looks Good

Uh oh!

github-actions bot commented Apr 20, 2026

AI Code Review

Critical Issues

Moderate Issues

Minor Issues

What Looks Good

Recommendation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 20, 2026

AI Code Review

Uh oh!

github-actions bot commented Apr 20, 2026

AI Code Review

What Looks Good

Recommendation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

github-actions bot commented Apr 20, 2026

AI Code Review

Critical Issues

Moderate Issues

Minor Issues

What Looks Good

Recommendation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

github-actions bot commented Apr 20, 2026

retran commented Apr 19, 2026 •

edited

Loading